Drop reset_attempts and reset_heartbeat from UnpauseActivityExecution - #11393
Conversation
|
Claude finished @dandavison's task in 4m 49s —— View job Review complete
Two inline comments, both non-blocking: one on the test-table shape in What I verified:
Not done: I could not compile or run tests in this environment ( |
Unpause resumes an activity; it should not also mutate the attempt counter or discard the heartbeat checkpoint. An operator wanting either would look for it on Reset, which offers both. Ship the simple API first. Picks up temporalio/api-go simplify-activity-unpause, which removes the two fields from UnpauseActivityExecutionRequest. The deprecated UnpauseActivity keeps them, so TestActivityPauseApi_WithReset now runs only against that API. Removing the reset_attempts branch also fixes a defect it carried: unpauseDispatchTime skipped dispatchTimeForRetry when the flag was set, so an operator could defeat a retry backoff by pausing and unpausing. go build ./... && go vet -tags test_dep ./... && go test ./chasm/lib/activity/... Extend the unpause simplification to the workflow-activity API Drops reset_attempts and reset_heartbeat from the deprecated UnpauseActivity and from the unpause batch operation, so unpause means the same thing on every surface: it resumes an activity, and Reset is the operation that restarts it as if on its first attempt. workflow.UnpauseActivity loses both parameters with them. TestActivityPauseApi_WithReset goes too: no unpause API can restart attempts now, so the case it covered no longer exists. ActivityInfo.reset_heartbeats stays: it is written by the Reset path, which keeps its own reset_heartbeat flag. go build ./... && go vet -tags test_dep ./... && go test ./chasm/lib/activity/... ./service/history/api/unpauseactivity/... ./service/worker/batcher/... Revert "Extend the unpause simplification to the workflow-activity API" This reverts commit 8295082. update api commit
d7eec33 to
eb47bd7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pause func(ctx context.Context, s *testcore.TestEnv, wfID, actID, identity, reason, requestID string) error | ||
| unpause func(ctx context.Context, s *testcore.TestEnv, wfID, actID, identity string, resetAttempts bool) error | ||
| unpause func(ctx context.Context, s *testcore.TestEnv, wfID, actID, identity string) error | ||
| // unpauseResettingAttempts is nil on an API with no reset_attempts flag. Only the deprecated | ||
| // UnpauseActivity has one; UnpauseActivityExecution deliberately does not. |
There was a problem hiding this comment.
Now that only one of the two APIs supports reset_attempts, the table gains a second func field, a nil check, a t.Skip, and a comment — all to express "this subtest only applies to the legacy API". Hoisting TestActivityPauseApi_WithReset out of the for _, api := range pauseAPIs() loop and having it call PauseActivity/UnpauseActivity directly would delete all four. The subtest body already builds its own env, workflow func and activity func; the only things it takes from api are pause and the unpause adapter, so nothing is shared that would need to be duplicated.
That also avoids the slightly odd shape where a subtest inside the UnpauseActivityExecution group exists only to be skipped.
Minor, on the comment itself: deliberately does not refers to the decision rather than the behavior. If you keep the field, something like // unpauseResettingAttempts is nil for APIs without a reset_attempts flag on unpause. says the same thing without it.
There was a problem hiding this comment.
Declined. That would cause a large diff to the WFA test, and this PR is about SAA not WFA.
| Activity: &workflowservice.UnpauseActivityRequest_Id{Id: frontendReq.GetActivityId()}, | ||
| Jitter: frontendReq.GetJitter(), | ||
| Identity: frontendReq.GetIdentity(), |
There was a problem hiding this comment.
Not introduced here, but this change makes it the whole story for the API, so worth confirming it's intended: the two backends of UnpauseActivityExecution now disagree on retry-backoff timing.
- Workflow activity (this branch):
workflow.UnpauseActivityregenerates the retry task atnow(+jitter)whenever the activity isSCHEDULED(service/history/workflow/activity.go:401-410), independent of the reset flags — so unpause drops the remaining backoff and dispatches immediately. - Standalone activity:
unpauseDispatchTimenow always consultsdispatchTimeForRetry, so unpause waits out the remaining backoff.tests/activity_standalone_test.go:11095asserts exactly that ("unpause must honor the remaining retry backoff").
Concretely: pause an activity sitting in a 30s retry backoff, then unpause. A workflow activity runs right away; a standalone activity waits for the original retry deadline. Previously reset_attempts=true gave the standalone path the workflow path's timing; with the flag gone, the only way to skip the backoff on a standalone activity is ResetActivityExecution, which also resets the attempt count and clears heartbeat details.
If the intent is that unpause is purely "resume", the legacy workflow path is the odd one out and may deserve a follow-up issue.
There was a problem hiding this comment.
This is something to fix in the workflow activity parity drive post saa release
There was a problem hiding this comment.
This is intended: we will bring WFA into parity with SAA later. Neither have this API at GA.
| Activity: &workflowservice.UnpauseActivityRequest_Id{Id: frontendReq.GetActivityId()}, | ||
| Jitter: frontendReq.GetJitter(), | ||
| Identity: frontendReq.GetIdentity(), |
There was a problem hiding this comment.
This is something to fix in the workflow activity parity drive post saa release
main dropped reset_heartbeat from UnpauseActivityExecution (#11393), so the UnpauseWithResetHeartbeat test goes away; keep this branch's UnpauseWhileCancelRequestedFails rename and FailedPrecondition assertions. go test ./tests/ -run TestActivityStandaloneSuite
4 participants
See API change temporalio/api#846
What changed?
reset_attemptsandreset_heartbeatfromUnpauseActivityExecutionWhy?
Unpause[resetAttempts]is received during retry backoff it is unclear whether to honor the remaining delay time, because this is how Unpause usually behaves, or dispatch immediately, because this is how Reset behaves.UnpauseandResetfunctionality in a confusing wayHow did you test it?
Breaking changes
Note
Medium Risk
Changes activity unpause scheduling semantics for the execution API and drops reset-on-unpause behavior that was only partially implemented; low user impact if the API was not GA and had no known callers.
Overview
Aligns the server with the UnpauseActivityExecution API change:
reset_attemptsandreset_heartbeatare no longer part of unpause for standalone (CHASM) activities.CHASM activity unpause no longer resets attempt count, retry interval, or heartbeat state on unpause, and always considers the pending retry backoff when scheduling dispatch (the branch that skipped that when
reset_attemptswas set is removed). Workflow-embedded unpause forwarding viaUnpauseActivityExecutionno longer passes those fields to the legacyUnpauseActivityhistory call (jitter and identity only).go.temporal.io/apiis bumped to the revision that removes the fields fromUnpauseActivityExecutionRequest.Tests are updated so unpause helpers no longer take a reset flag; reset-on-unpause coverage stays on legacy
UnpauseActivityonly (execution API skips that case). Standalone tests forUnpauseWithResetAttemptsandUnpauseWithResetHeartbeatonUnpauseActivityExecutionare removed.Reviewed by Cursor Bugbot for commit 7a41d50. Bugbot is set up for automated code reviews on this repo. Configure here.